Allow retries on Vault MP Config Source initialization #20343
Conversation
|
not ready to merge. I am reworking the impl. |
|
hello @geoand . thanks for the initial review. there were a few shortcomings I realized and wanted to address:
so I changed the design to translate the different error conditions into a common exception I also changed the default read timeout to be does this sound reasonable? thanks for your time. |
vsevel
added
the
triage/waiting-for-ci
|
I'll let @sberyozkin review this as well |
geoand
removed
the
triage/waiting-for-ci
|
hi @sberyozkin how are you? any chance you could take a look? thanks. |
|
hi @sberyozkin do you think you will be able to take a look? or if you do not have time, would you know somebody who may be able to do it? thanks. |
|
Hi Vincent @vsevel very sorry, I keep missing many pings, please ping me on Zulip if I don't respond, if I don't it means I have missed the ping :-) |
|
Let me do it right now... |
| // happens if we reach the atMost condition - see UniAwait.atMost(Duration) | ||
| throw new VaultIOException(e); | ||
|
|
||
| } catch (VertxException e) { |
There was a problem hiding this comment.
@vsevel in quarkus-oidc we add a few retries in such cases, so please consider it, but it is not needed for this PR
There was a problem hiding this comment.
initially I had the retry logic in the vault client itself. but I did not want to apply it to all calls. so I started passing args around to control when and when not to retry. to avoid changing to much the behavior (introducing lags instead of errors), I wanted to limit it to the very first time we fetch properties out of the vault, because it is in the critical path for startup. we will see if this needs to be added in other use cases.
|
@vsevel It looks fine, simple and logical improvement :-), sorry for keeping you waiting |
hi @sberyozkin I figured you had other things to do too ;-) |
During the initialization of the Vault Config Source, vault (or one specific replica) might be temporarily unavailable.
In that case the rest call through the vertx web client will throw a mutiny timeout exception, which will make the application fail at startup.
Retrying, sometimes just once, may be enough to successfully fetch the secrets from the Vault (or a good replica) after an initial failure, which will allow for the application to continue its initialization, and avoid a crash and a restart.
Ideally the remote service should always be up. But this can't be guaranteed, and since it is on the critical path of application startup, it is fair to add some resiliency.
This PR provides a way to allow some retries during the very first time secrets get fetched by the vault config source.
Once fetched successfully once, the retries will not be used again. Instead we will rely on the internal cache.
Retries are used only in the context of the vault config source. In particular it is not being used by the different programmatic interfaces for the supported engines.
To summarize it is only used by the vault config source, and only on the very first access.